Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add animated class to the dotted links #5650

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

cmnstmntmn
Copy link

@cmnstmntmn cmnstmntmn commented Jul 17, 2024

📑 Summary

The purpose of this PR is to add more clarity to a
diagram and to how the nodes of a diagram communicate

Adding animate class to a node, will add the
animated class to an edge of type arrow-point
having stroke dotted

Eg.

graph LR
A:::producer -.-> B:::consumer -.-> A -.-> C

Resolves #5574

📏 Design Decisions

Since creating keyframes animations was not possible,
a .producer / .consumer class pair has been added to the project.

It can be used only on dotted links.
Eg.

graph LR
A:::producer -.-> B:::consumer -.-> A -.-> C

animated-producer-consumer

📋 Tasks

Make sure you

@github-actions github-actions bot added the Type: Enhancement New feature or request label Jul 17, 2024
Copy link

netlify bot commented Jul 17, 2024

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 2455af1
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/66a3d11dfbc04100080bc446
😎 Deploy Preview https://deploy-preview-5650--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.

Project coverage is 5.85%. Comparing base (c0bf6d8) to head (2455af1).
Report is 1166 commits behind head on develop.

Files with missing lines Patch % Lines
.../mermaid/src/diagrams/flowchart/flowRenderer-v2.js 0.00% 13 Missing ⚠️
packages/mermaid/src/diagrams/flowchart/styles.ts 0.00% 9 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #5650      +/-   ##
==========================================
- Coverage     5.85%   5.85%   -0.01%     
==========================================
  Files          274     274              
  Lines        41112   41132      +20     
  Branches       488     488              
==========================================
  Hits          2408    2408              
- Misses       38704   38724      +20     
Flag Coverage Δ
unit 5.85% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/mermaid/src/diagrams/flowchart/styles.ts 0.00% <0.00%> (ø)
.../mermaid/src/diagrams/flowchart/flowRenderer-v2.js 0.00% <0.00%> (ø)

Copy link

argos-ci bot commented Jul 17, 2024

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ✅ No changes detected - Jul 26, 2024, 4:50 PM

Copy link
Member

@sidharthv96 sidharthv96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks SO cool!! Great work 🚀

To give you more context into our syntax design philosophy, our core principle is that a change we introduce shouldn't break existing features. So we tend to have a thorough process whenever there are syntax changes. As internal implementations are easier to change without affecting users, those get little less scrutiny.

I was playing around with it, and there is a small issue I noticed. We are adding the :::animated class on a node, not an edge. That means, all the edges originating from that node will be animated automatically.
We should brainstorm to find a way to select individual edges if possible.

I really love the simplicity of the current syntax. So maybe we can keep that, and add something to make individual edges animated in a later release.
Another useful feature would be a config option, which would enable animation for the whole diagram.

---
config:
  animated: true
---
<diagram>

Btw, all these are not blockers for this PR.

packages/mermaid/src/diagrams/flowchart/flowRenderer-v2.js Outdated Show resolved Hide resolved
@cmnstmntmn cmnstmntmn force-pushed the feature/5574_add-animated-links branch from 0a6201f to 84b10ac Compare July 19, 2024 10:27
@cmnstmntmn cmnstmntmn requested a review from sidharthv96 July 19, 2024 10:38
@cmnstmntmn
Copy link
Author

cmnstmntmn commented Jul 19, 2024

@cmnstmntmn cmnstmntmn force-pushed the feature/5574_add-animated-links branch 2 times, most recently from f468686 to 2d9682e Compare July 21, 2024 09:32
The purpose of this PR is to add more clarity to a
diagram and to how the nodes of a diagram communicate

Adding `producer`/`consumer` pair classes to nodes,
will add the `animated` class to an egde ot type 'arroe-point'
having stroke 'dotted'

Eg.

```mermaid
graph LR
A:::producer -.-> B:::consumer -.-> A -.-> C
```
@cmnstmntmn cmnstmntmn force-pushed the feature/5574_add-animated-links branch from 2d9682e to 22d0f40 Compare July 23, 2024 10:42
@sidharthv96
Copy link
Member

@cmnstmntmn This kinda breaks the simplicity we had with the :::animate class. The current approach adds more control, but feels clunkier. Also, the producer, consumer classes might be common and could interfere with existing diagrams.

We should brainstorm to find a way to select individual edges if possible.

I really love the simplicity of the current syntax. So maybe we can keep that, and add something to make individual edges animated in a later release.

What if, we add :::animate to both sides (node) of an edge, instead of producer and consumer? It will take away a bit of control, but the syntax is easier to remember.

Just to be clear, I want your opinion on this, please don't write code 😅, we can discuss and finalise an approach and then write code. 😇

graph LR
A:::animate -.-> B:::animate -.-> A -.-> C

And maybe if we play around with the parsing a bit, we might be able to isolate just the edge that needs to be animated. I can help with that part.

@cmnstmntmn
Copy link
Author

yeap, having .animate on both ends seems easier and more generic

@knsv
Copy link
Collaborator

knsv commented Aug 5, 2024

Hello! Thanks @cmnstmntmn for pitching in!
There is a general problem with edges in Mermaid. Using the current syntax, you cannot assign an ID to the actual edge. You need an ID to be able to play styling/classes to the specific edge at later statements.

This has led us to use the order of appearance for applying styles to edges. This approach comes with its own set of problems. If you introduce a new edge that can shuffle styling around, this approach could be used for animation as well.

In your example, @sidharthv96, with classes applied to both nodes as selectors, we will run into trouble when there is more than one edge and we want to apply the style to one of the edges.

I think it is time to solve this more robustly. I suggest that we introduce a way to attach an ID to an edge using the syntax itself without relying on work-arounds.

I am not saying this change needs to be done in this PR, but it should be the way to select an edge for styling.

Some spitballing - It could look something like this:

...
C{"Evaluate"} -- One {id: one} --> D

Where the label ends with the id statement we set the id of the label in the db when creating the label. Then we can use the class statement perhaps:

class one animate

We could also allow classes directly in the id statement:

...
C{"Evaluate"} -- One {id: one, class: animate} --> D

would work with style statements as well:

linkStyle one stroke:#2962FF
%% same thing as below but without the risk of reshuffling
linkStyle 0 stroke:#2962FF

We need to think through the actual syntax in the edge label, as once we introduce it, we are stuck with it. My own objection to {id: one, class: animate} is that it looks very similar to a decision making the diagram harder to read. Something along those lines would be good though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow linkStyle to be animated
3 participants